feat(mcp): adopt agent's newer copies of the eleven shared Core\Mcp classes - #20
Conversation
…lasses
Step 1 of letting dappcore/agent consume this package instead of forking it.
Eleven classes exist in both repos under the same Core\Mcp namespace, because
agent maps Core\Mcp\ onto its own php/Mcp/ as well. The public API is identical
on all eleven — same methods, same constants — and every difference is in
protected helpers: this package prefixes them with get (getStateKey,
getCacheKey, getRegisteredServers) where agent does not, and agent extracts a
few more.
Provenance settles which way to merge. These files date from this package's
2026-03-09 initial commit; agent's landed 2026-04-25 as "implement extended RFC
services + transport" and were then hardened on 2026-04-27 by "address
CodeRabbit + SonarCloud findings". Agent's are the downstream, newer, reviewed
copies, so they come upstream rather than being overwritten by ours — SPOR
with best-version-wins, not first-writer-wins.
Chased the one candidate for the other direction: OpenApiGenerator::
buildSchemas exists only here. It is not lost — agent emits the identical
schemas block inline inside buildComponents rather than through an extracted
helper. No behaviour only this side had.
AX-8 verified before copying, not assumed: none of the eleven imports
Core\Mod\Agentic. Their only non-framework imports are Core\Tenant,
Core\Mod\Content and Laravel\Mcp, all of which this package already uses.
php-content is now declared for the first time — ContentResource has always
imported Core\Mod\Content\Models\ContentItem without it being in composer.json.
CircuitBreaker arrives with its namespace normalised from the braced
`namespace X { ... }` form, which was the only file in either repo using it.
One correction to #19, which I got wrong. That PR was titled "stop importing
the consumer" and it introduced one: running pint over the changed files let
fully_qualified_strict_types rewrite the inline
\Core\Mod\Agentic\Services\AgentToolRegistry::class in executeTool into a
top-of-file use statement. The coupling never changed — it is still a guarded,
late-bound app()->bound() lookup that autoloads nothing — but the source did,
and the claim in that PR body was wrong as written. It is now a string literal,
which expresses the late binding honestly and which no formatter will promote
into an import.
SPDX headers come across with the files: 13 of 132 files now carry one, from 2.
Suite unchanged: 21 failed, 299 passed.
Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 57 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TestBridge_NewBridge_Bad has failed on main since 1523144, which made NewBridge apply cfg.WithDefaults() so a zero Config cannot leave ReconnectInterval at 0 — connectLoop's min(delay*2, max) backoff would stay pinned at zero and reconnect in a tight loop. The fix was right. The test was left asserting the behaviour the fix removed: AssertEqual(t, "", bridge.cfg.LaravelWSURL) want="" got="ws://localhost:9876/ws" So the assertion has been describing the bug rather than the contract, and every push to main since has been red on it. It now asserts what NewBridge guarantees: a zero Config comes back defaulted, with a URL, a workspace root, and both reconnect intervals non-zero — the property the change existed to establish, rather than the one string it happened to set. Not caused by #20, which is red for this and touches no Go at all: its fourteen files are php/ plus composer.json and composer.lock. go test ./... now passes for the whole module.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Step 1 of 3 in letting
dappcore/agentconsume this package instead of forking it. Step 2 is agent taking the composer dependency and deleting its local copies; step 3 turns the agent MCP server on.Why these, and why this direction
Eleven classes exist in both repos under the same
Core\Mcpnamespace, because agent mapsCore\Mcp\onto its ownphp/Mcp/too. Public API is identical on all eleven — same methods, same constants. Every difference is protected helpers: this package prefixes them withget(getStateKey,getCacheKey,getRegisteredServers) where agent doesn't, and agent extracts a few more.Provenance settles the direction:
Agent's are the downstream, newer, reviewed copies. So they come upstream rather than being overwritten by ours — SPOR with best-version-wins, not first-writer-wins.
I chased the one candidate for the opposite direction:
OpenApiGenerator::buildSchemasexists only here. Not lost — agent emits the identical schemas block inline insidebuildComponentsrather than through an extracted helper. Nothing this side had that agent doesn't.AX-8 verified before copying
None of the eleven imports
Core\Mod\Agentic. Their only non-framework imports areCore\Tenant,Core\Mod\ContentandLaravel\Mcp— all already used here.php-contentis now declared for the first time:ContentResourcehas always importedCore\Mod\Content\Models\ContentItemwithout it appearing incomposer.json.CircuitBreakerarrives with its namespace normalised from the bracednamespace X { … }form — the only file in either repo using it.A correction to #19
That PR was titled "stop importing the consumer" and it introduced one. Running pint over the changed files let
fully_qualified_strict_typesrewrite the inline\Core\Mod\Agentic\Services\AgentToolRegistry::classinexecuteToolinto a top-of-fileuse.The coupling never changed — it's still a guarded, late-bound
app()->bound()lookup that autoloads nothing — but the source did, and the claim in that PR body was wrong as written. It's now a string literal, which expresses the late binding honestly and which no formatter will promote into an import.Verification
mainCore\Mod\Agenticimport anywhere inphp/src/after linting (checked post-pint, given the above)🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io